Revisit importTable() in Consensus Commit #3102

brfrn169 · 2025-10-30T15:01:51Z

Description

This PR revisits the logic of the importTable() method in Consensus Commit.

Related issues and/or PRs

N/A

Changes made

Refactored the logic of the ConsensusCommitAdmin.importTable() method to call admin.importTable() first, then add columns using addNewColumnToTable() instead of addRawColumnToTable().
Removed the getImportTableMetadata() and addRawColumnToTable() methods from the admin API interfaces and all implementations
Removed associated tests for the deleted methods

Checklist

I have commented my code, particularly in hard-to-understand areas.
I have updated the documentation to reflect the changes.
I have considered whether similar issues could occur in other products, components, or modules if this PR is for bug fixes.
Any remaining open issues linked to this PR are documented and up-to-date (Jira, GitHub, etc.).
Tests (unit, integration, etc.) have been added for the changes.
My changes generate no new warnings.
Any dependent changes in other PRs have been merged and published.

Additional notes (optional)

N/A

Release notes

N/A

gemini-code-assist · 2025-10-30T15:02:17Z

Summary of Changes

Hello @brfrn169, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly refactors the importTable() functionality within the Consensus Commit transaction layer. The primary goal is to simplify the DistributedStorageAdmin interface by removing methods that were previously used for granular control over table import and column addition. The ConsensusCommitAdmin's importTable method has been revised to delegate the core table import process to the underlying storage implementation, then programmatically add necessary transaction metadata and before-image columns. This change promotes a cleaner separation of concerns and a more robust approach to table importing in a transactional context.

Highlights

Simplified DistributedStorageAdmin Interface: The getImportTableMetadata and addRawColumnToTable methods have been removed from the DistributedStorageAdmin interface, streamlining the public API.
Refactored ConsensusCommitAdmin.importTable() Logic: The importTable method in ConsensusCommitAdmin now directly utilizes the underlying storage admin's importTable functionality and then adds transaction-specific columns using addNewColumnToTable, removing previous table existence checks and metadata repair calls.
Internalization of Admin Methods: Methods like getImportTableMetadata and addRawColumnToTable are no longer exposed publicly, with JdbcAdmin making getImportTableMetadata @VisibleForTesting.
Updated Batch Read-Only Check: The batch method in TwoPhaseConsensusCommitManager and JdbcTransactionManager now uses o instanceof Selection for determining read-only operations, aligning with a more general Selection interface.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request removes the getImportTableMetadata and addRawColumnToTable methods from the DistributedStorageAdmin interface and related classes, as well as disables corresponding tests for Cassandra and DynamoDB, and modifies the importTable method in ConsensusCommitAdmin. The review focuses on the correctness of removing these methods and the impact on the overall system.

core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitAdmin.java

.../src/main/java/com/scalar/db/transaction/consensuscommit/TwoPhaseConsensusCommitManager.java

Copilot

Pull Request Overview

This PR refactors the import table functionality by removing deprecated getImportTableMetadata and addRawColumnToTable methods from the DistributedStorageAdmin interface and its implementations, simplifying the import process to use importTable directly followed by addNewColumnToTable for metadata columns.

Key changes:

Removed getImportTableMetadata and addRawColumnToTable methods from the admin API interfaces and all implementations
Updated ConsensusCommitAdmin.importTable to call admin.importTable() first, then add columns using addNewColumnToTable instead of addRawColumnToTable
Refactored batch operation read-only checks to use instanceof Selection instead of checking for Get or Scan individually
Removed associated tests for the deleted methods

Reviewed Changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
`core/src/main/java/com/scalar/db/api/DistributedStorageAdmin.java`	Removed `getImportTableMetadata` and `addRawColumnToTable` method declarations from interface
`core/src/main/java/com/scalar/db/common/CommonDistributedStorageAdmin.java`	Removed wrapper implementations for deleted methods
`core/src/main/java/com/scalar/db/service/AdminService.java`	Removed delegation implementations for deleted methods
`core/src/main/java/com/scalar/db/storage/multistorage/MultiStorageAdmin.java`	Removed routing implementations for deleted methods
`core/src/main/java/com/scalar/db/storage/jdbc/JdbcAdmin.java`	Removed `addRawColumnToTable` implementation and changed `getImportTableMetadata` visibility to package-private
`core/src/main/java/com/scalar/db/storage/dynamo/DynamoAdmin.java`	Removed unsupported operation stubs for deleted methods
`core/src/main/java/com/scalar/db/storage/cosmos/CosmosAdmin.java`	Removed unsupported operation stubs for deleted methods
`core/src/main/java/com/scalar/db/storage/cassandra/CassandraAdmin.java`	Removed unsupported operation stubs for deleted methods
`core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitAdmin.java`	Refactored `importTable` to call `admin.importTable()` first, then use `addNewColumnToTable` instead of `addRawColumnToTable`
`core/src/main/java/com/scalar/db/transaction/jdbc/JdbcTransactionManager.java`	Simplified batch read-only check to use `instanceof Selection`
`core/src/main/java/com/scalar/db/transaction/consensuscommit/TwoPhaseConsensusCommitManager.java`	Simplified batch read-only check to use `instanceof Selection`
`core/src/test/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitAdminTestBase.java`	Updated test to verify `importTable` is called before adding columns, changed assertions to use `addNewColumnToTable`
`core/src/test/java/com/scalar/db/storage/multistorage/MultiStorageAdminTest.java`	Removed test for deleted `getImportTableMetadata` method
`core/src/test/java/com/scalar/db/storage/jdbc/JdbcAdminTest.java`	Removed tests for deleted `addRawColumnToTable` method
`core/src/test/java/com/scalar/db/storage/dynamo/DynamoAdminTestBase.java`	Removed unsupported operation tests for deleted methods, renumbered remaining test variables
`core/src/test/java/com/scalar/db/storage/cosmos/CosmosAdminTest.java`	Removed unsupported operation tests for deleted methods, renumbered remaining test variables
`core/src/test/java/com/scalar/db/storage/cassandra/CassandraAdminTest.java`	Removed unsupported operation tests for deleted methods, renumbered remaining test variables
`integration-test/src/main/java/com/scalar/db/api/DistributedStorageAdminPermissionIntegrationTestBase.java`	Removed integration tests for deleted methods
`core/src/integration-test/java/com/scalar/db/storage/dynamo/DynamoAdminPermissionIntegrationTest.java`	Removed disabled test overrides for deleted methods
`core/src/integration-test/java/com/scalar/db/storage/cassandra/CassandraAdminPermissionIntegrationTest.java`	Removed disabled test overrides for deleted methods
`core/src/integration-test/java/com/scalar/db/storage/jdbc/JdbcSchemaLoaderImportIntegrationTest.java`	Added missing `@AfterAll` annotation to `afterAll` method

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitAdmin.java

core/src/main/java/com/scalar/db/api/DistributedStorageAdmin.java

komamitsu

LGTM! 👍

Torch3333

LGTM, thank you!

core/src/main/java/com/scalar/db/storage/jdbc/JdbcAdmin.java

feeblefakie

LGTM! Thank you!

jnmt

LGTM, thank you!

core/src/main/java/com/scalar/db/storage/jdbc/JdbcAdmin.java

Revisit importTable() in Consensus Commit

600c743

brfrn169 self-assigned this Oct 30, 2025

Copilot AI review requested due to automatic review settings October 30, 2025 15:01

brfrn169 added the improvement label Oct 30, 2025

gemini-code-assist bot reviewed Oct 30, 2025

View reviewed changes

core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitAdmin.java Show resolved Hide resolved

.../src/main/java/com/scalar/db/transaction/consensuscommit/TwoPhaseConsensusCommitManager.java Show resolved Hide resolved

Copilot AI reviewed Oct 30, 2025

View reviewed changes

core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitAdmin.java Show resolved Hide resolved

brfrn169 commented Oct 31, 2025

View reviewed changes

core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitAdmin.java Show resolved Hide resolved

core/src/main/java/com/scalar/db/api/DistributedStorageAdmin.java Show resolved Hide resolved

brfrn169 requested review from Torch3333, feeblefakie, jnmt and komamitsu October 31, 2025 04:24

Fix

0393656

komamitsu approved these changes Nov 4, 2025

View reviewed changes

Torch3333 approved these changes Nov 4, 2025

View reviewed changes

jnmt reviewed Nov 4, 2025

View reviewed changes

core/src/main/java/com/scalar/db/storage/jdbc/JdbcAdmin.java Show resolved Hide resolved

feeblefakie approved these changes Nov 4, 2025

View reviewed changes

brfrn169 requested a review from jnmt November 5, 2025 01:30

jnmt approved these changes Nov 5, 2025

View reviewed changes

core/src/main/java/com/scalar/db/storage/jdbc/JdbcAdmin.java Show resolved Hide resolved

Merge branch 'master' into revisit-importTable-in-consensus-commit

71c817d

brfrn169 merged commit aac0d47 into master Nov 5, 2025
66 checks passed

brfrn169 deleted the revisit-importTable-in-consensus-commit branch November 5, 2025 04:39

feeblefakie mentioned this pull request Nov 5, 2025

Backport to branch(3) : Revisit importTable() in Consensus Commit #3114

Merged

brfrn169 added a commit that referenced this pull request Nov 5, 2025

Revisit importTable() in Consensus Commit (#3102)

fcd5288

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Revisit importTable() in Consensus Commit #3102

Revisit importTable() in Consensus Commit #3102

Uh oh!

brfrn169 commented Oct 30, 2025 •

edited

Loading

Uh oh!

gemini-code-assist bot commented Oct 30, 2025

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

komamitsu left a comment

Uh oh!

Torch3333 left a comment

Uh oh!

Uh oh!

feeblefakie left a comment

Uh oh!

jnmt left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Revisit importTable() in Consensus Commit #3102

Revisit importTable() in Consensus Commit #3102

Uh oh!

Conversation

brfrn169 commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues and/or PRs

Changes made

Checklist

Additional notes (optional)

Release notes

Uh oh!

gemini-code-assist bot commented Oct 30, 2025

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

komamitsu left a comment

Choose a reason for hiding this comment

Uh oh!

Torch3333 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

feeblefakie left a comment

Choose a reason for hiding this comment

Uh oh!

jnmt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

brfrn169 commented Oct 30, 2025 •

edited

Loading